Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/Create Hardcoded API with Schema #3

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

leonardostsouza
Copy link
Contributor

No description provided.

@leonardostsouza leonardostsouza changed the title WIP: Feat/Create Hardcoded API with Schema Feat/Create Hardcoded API with Schema Oct 1, 2019
Copy link

@bamorim bamorim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec is not as we discussed.
Also, the API is not set correctly (content-type wise).
Also, some of the responses are weird (returning 201 on a failed request is a completely no-no)

src/routes/api-routes.js Outdated Show resolved Hide resolved
src/routes/api-routes.js Outdated Show resolved Hide resolved
src/routes/api-routes.js Show resolved Hide resolved
src/routes/api-routes.js Outdated Show resolved Hide resolved
src/routes/api-routes.js Show resolved Hide resolved
src/routes/api-routes.js Outdated Show resolved Hide resolved
src/routes/api-routes.js Show resolved Hide resolved
src/routes/api-routes.js Outdated Show resolved Hide resolved
src/routes/api-routes.js Outdated Show resolved Hide resolved
Copy link

@bamorim bamorim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, it looks better. The only problem I think is the content type.
So adding the content type assertion to the tests is nice. Also, moving the utils and event store to a diferent file with corresponding unit tests (not passing through the API) would improve things.

Summing up

  • Add assertions about content-type
  • Move isValidSize and isBase64 to utils and add unit tests to them.
  • Move event store related to something like event_store/in_memory.js and adding unit tests.

src/routes/api-routes.js Outdated Show resolved Hide resolved
src/routes/api-routes.js Outdated Show resolved Hide resolved
src/routes/api-routes.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants